Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add build task experimental rules #1307

Merged

Conversation

paul-dingemans
Copy link
Collaborator

Description

Ktlint should apply the dogfooding principal and only provide experimental rules that at least on the ktlint code base itself gives satisfiable results.

In the intial commit of the PR all experimental rules that causes a violation have been disabled. In following commits, rules are enabled one by one to make the consequences of enabling a rule clear. See commit messages for further details about choices made. Most important choices:

  • experimental rules should always be enforced on the ktlint code base itself. If we are not happy with the effect of a rule on our own code base, we should not provide such a rule
  • The experimental:argument-list-wrapping is the only rule which is not yet enabled as it breaks on some false
    positives. This will be solved by Remove lint warning about unexpected indentation outside of indent rule #1284 after which this rule can and should be enabled as well.
  • For now, it has been chosen to disallow trailing comma's instead of forcing them to be added. I am also fine with enabling the rules. But this results in a lot of changes (145 files). It will be best to change this directly after merging this PR into master.

Checklist

  • tests are added
  • CHANGELOG.md is updated

Paul Dingemans added 9 commits December 12, 2021 10:09
Ktlint should apply the dogfooding principal and only provide experimental
rules that at least on the ktlint code base itself gives satisfiable
results.

Initially all experimental rules that cause violations have been disabled,
so that a separate commit can be created to enable each specific rule.
For BaselineTests it was necessary to rename the files which are used for testing
to have a non standard kotlin file extension. This prevents the files from being
changed when running the ktlint formatting on the ktlint code base itself. Note
that the baseline protection mechanism did work in this case and as of that has
been removed from the command.
It is not yet possible to enable the rule as some violations are actually false
positives. This will be solved by pinterest#1284
For now, it has been chosen to disallow trailing comma's instead of forcing them to
be added. Reasons for this are two folded. The number of changes is considerably
smaller. More importantly is that the benefit, with respect to avoiding future merge
conflicts, seems not that big when scanning the code change which would result from
forcing the trailing comma to be added.
Copy link
Collaborator

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 minor things, but LGTM otherwise, thanks.

build.gradle Outdated Show resolved Hide resolved
ktlint/src/test/resources/TestBaselineFile._kt Outdated Show resolved Hide resolved
Paul Dingemans added 2 commits December 13, 2021 21:19
There is no need for a separate build task to run the experimental rules. The
experimental rules can be executed by default in the "ktlint" task. Also, the
baseline has been fixed so there is no longer a need to use extension "_kt"
for the baseline test files.

Closes pinterest#1222
romtsn and others added 8 commits December 14, 2021 09:01
…perimental-rules

# Conflicts:
#	CHANGELOG.md
#	build.gradle
Those source files all contain linting errors which have to be reported
by unit tests. Therefore they should not be reported during a normal
build because those should not be fixed as that would result in the
tests to fail.
@paul-dingemans
Copy link
Collaborator Author

@romtsn Can you give it another try? I have fixed the tests.

@romtsn romtsn merged commit 77c60e5 into pinterest:master Jan 21, 2022
@paul-dingemans paul-dingemans deleted the add-build-task-experimental-rules branch February 5, 2022 11:14
@chao2zhang chao2zhang mentioned this pull request Jul 24, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants